CMP-4435: Add manual rule kubevirt-restrict-migration-tools-access for CIS OCP-Virt 1.10 - #14920
Conversation
|
Hi @taimurhafeez. Thanks for your PR. I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
taimurhafeez
changed the title
taimurhafeez
changed the title
|
/retest |
|
@taimurhafeez: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
@taimurhafeez: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Verified the bundler change empirically: the build warns and skips the manual rule from CEL content correctly, and a nonexistent rule id in selections still hard-fails the build at the XCCDF compile stage, so no typo-safety is lost. Rule content matches the benchmark audit. This PR should be the single carrier of the build-script/docs change — the other manual-rule PRs can rebase down to rule + selection once it merges. /lgtm |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/lgtm |
yuumasato
left a comment
yuumasato
left a comment
There was a problem hiding this comment.
@Vincent056 How did you test it?
When I build the content, I don't see any mention of Rule kubevirt-restrict-migration-tools-access in build/ocp4-cel-content.yaml. The rule is not listed in the profile as well.
| logging.warning( | ||
| "profile '%s' references rule '%s' without CEL checks " | ||
| "(manual rule) - skipping from CEL content", | ||
| profile_name, rule_name, |
There was a problem hiding this comment.
@taimurhafeez the unit tests are failing as it expects an exception.
As it is, this function doesn't differentiate a non-existing rule from a rule that doesn't have a CEL check, I think generate_cel_content now needs to be passed a list of all existing rules, so that it knows whether a rule exists without a CEL check but is still intended to be addded to a CEL Profile, or it is a rule that doesn't exist.
There was a problem hiding this comment.
Basically, if a rule exists, but doesn't have a CEL check, we can raise a warning that it is a Manual rule and continue with the build.
But if the rule doesn't exist we keep raising the exception.
There was a problem hiding this comment.
@yuumasato I addressed this in a new commit.
|
@Vincent056 @taimurhafeez |
Add load_all_rule_ids() to collect all compiled rule IDs. Update generate_cel_content() to warn and skip when a CEL profile references an existing rule without CEL checks (manual rule), but still raise ValueError for rules that do not exist at all (typo protection). Update unit tests to match the new behavior.
|
The new commit is the right fix — warn-and-skip only for rules that exist without CEL checks, hard error kept for nonexistent ids, with the bundler unit tests updated to match. Re-verified the distinction logic in the diff. Note the failing /lgtm |
Add load_manual_rules() to load rules selected by CEL profiles that have no CEL checks. These are emitted into cel-content.yaml with checkType: Manual (no expression, no inputs) so the compliance-operator can produce MANUAL/notchecked results for them. Update generate_cel_content() and main() to collect and include manual rules alongside CEL rules in the output.
|
/retest |
| if rule_id in all_rule_ids: | ||
| logging.warning( | ||
| "profile '%s' references rule '%s' without CEL checks " | ||
| "(manual rule) - skipping from CEL content", |
There was a problem hiding this comment.
The rule is not skipped, it is added without the CEL expression and inputs.
| "(manual rule) - skipping from CEL content", | |
| "- Adding as manual rule", |
There was a problem hiding this comment.
We already identify that a rule is Manual here.
Could we process profiles before rules?
And add the manual rules found here into cel_rules_list? Or some renamed variable that aggregates all the rules that need to be dumped into the output file?
| for rule_id in profile.selected: | ||
| if rule_id not in cel_rule_ids and rule_id in all_rule_ids: | ||
| rule = all_rules[rule_id] | ||
| rule.check_type = 'Manual' |
There was a problem hiding this comment.
I don't agree with the Manual check type.
The check type is about what is being checked, whether the kubernetes resource in the Platform or the file or service configuration in the Node.
A Manual rule is just missing the automated check.
There was a problem hiding this comment.
We don't need to filter out manual CEL rules here, they can be identified directly inside generate_cel_content().
Any rule, including the existing SCAP rules, that miss a CEL check is a Manual rule in a CEL Profile.
| def load_rules(rules_dir): | ||
| """ | ||
| Load all rules that use the CEL checking engine. | ||
| Load all compiled rules, separating CEL rules from the rest. |
There was a problem hiding this comment.
| Load all compiled rules, separating CEL rules from the rest. | |
| Load all compiled rules, separating rules with CEL check from the rest. |
|
|
||
| # Check if this rule has CEL checks by looking for CEL-specific fields | ||
| # A rule uses CEL if it has both expression and inputs | ||
| # (loaded from cel/shared.yml during rule compilation) |
There was a problem hiding this comment.
Why are these comments removed?
| profiles: List of profiles targeting the CEL checking engine | ||
| all_rule_ids: Set of all compiled rule IDs (used to distinguish | ||
| manual rules from nonexistent rules) | ||
| manual_rules: Dictionary of manual rules (rules without CEL checks |
There was a problem hiding this comment.
It is not clear to me why we need manual_rules here.
| for profile in profiles: | ||
| for rule_id in profile.selected: |
There was a problem hiding this comment.
generate_cel_content() already has two nested loops like this one, in there it could check whether a selected rule is Manual (i.e.: not part of cel_rules but exists in all_rules_ids).
|
|
||
| def test_validation_empty_inputs(): | ||
| """Test that rule with empty inputs list is skipped.""" | ||
| """Test that rule with empty inputs list is skipped from CEL rules.""" |
There was a problem hiding this comment.
Comment is not accurate anymore, the rule is included in the CEL outptu.
|
|
||
| def test_validation_mixed_oval_and_cel_in_profile(): | ||
| """Test that profile with both OVAL and CEL checks only includes rules with CEL checks.""" | ||
| """Test that profile with both OVAL and CEL checks only includes CEL rules in output.""" |
There was a problem hiding this comment.
| """Test that profile with both OVAL and CEL checks only includes CEL rules in output.""" | |
| """Test that profile with both OVAL and CEL checks are output as CEL and manual rules.""" |
| ``` | ||
|
|
||
| **Important:** CEL profiles can only select CEL rules. If a profile includes both CEL and OVAL rules, only the CEL rules will be included in the generated CEL content file. | ||
| **Important:** CEL profiles can select both CEL rules and manual rules (rules without `cel/shared.yml`). Only CEL rules are included in the generated CEL content file; manual rules are skipped with a build warning. |
There was a problem hiding this comment.
| **Important:** CEL profiles can select both CEL rules and manual rules (rules without `cel/shared.yml`). Only CEL rules are included in the generated CEL content file; manual rules are skipped with a build warning. | |
| **Important:** CEL profiles can select both CEL and SCAP rules. If a CEL rules doesn't have manual rules (rules without `cel/shared.yml`), or a SCAP rule is selected it will be included as manual rule with a build warning. |
|
@taimurhafeez: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Adds a manual rule to restrict namespace administrator access to migration tools (vmim and migrationpolicy resources) for the CIS VM Extension Benchmark. The rule documents the audit procedure without an automated CEL check, as the pass/fail is a human authorization decision.